Skip to content

feat(Async): RunSynchronouslyImmediate#19804

Open
bartelink wants to merge 4 commits into
dotnet:mainfrom
bartelink:run-synchronously-immediate
Open

feat(Async): RunSynchronouslyImmediate#19804
bartelink wants to merge 4 commits into
dotnet:mainfrom
bartelink:run-synchronously-immediate

Conversation

@bartelink
Copy link
Copy Markdown

@bartelink bartelink commented May 25, 2026

Implements RunSynchronouslyImmediate per fsharp/fslang-suggestions#1042

See also fsharp/fslang-suggestions#1467

NOTE the tests (and especially the comments within them) are mainly generated. Similarly the xmldoc updates are a best effort mix of generated content combined with me attempting to increase chances that an intellisense reader will come to the right conclusions in terms of tradeoff between potential deadlock vs better stacktraces. As such, please feel free to relentlessly criticize and/or rewrite to your hearts content.

image image

Checklist

  • Needs a killer example of the stacktrace difference between Async.RS vs Async.RSI
    • One aspect should potentially be represented as a regression test
    • More importantly, if the xmldoc summary can shout out the difference / why one should consider deadlocking, that's key
  • Test cases added
    • validate absence of thread hops
    • characterize stacktrace that can be expected (perhaps contrasting with RunSynchronously)
  • Release notes entry updated:

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/11.0.100.md

@bartelink bartelink force-pushed the run-synchronously-immediate branch from 181ce94 to 41f5f13 Compare May 25, 2026 18:44
@bartelink bartelink force-pushed the run-synchronously-immediate branch from d6a0470 to 69e41a2 Compare May 25, 2026 22:07
@bartelink bartelink marked this pull request as ready for review May 25, 2026 22:19
@bartelink bartelink requested a review from a team as a code owner May 25, 2026 22:19
Copilot AI review requested due to automatic review settings May 25, 2026 22:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Async.RunSynchronouslyImmediate to FSharp.Core to allow running an Async<'T> synchronously while always executing the initial step on the calling thread (aimed at improved diagnostics/stack traces in FSI/tests), with accompanying API surface updates, documentation, unit tests, and release notes.

Changes:

  • Add public API Async.RunSynchronouslyImmediate and wire it through Async primitives.
  • Add unit tests characterizing basic behavior and key differences vs Async.RunSynchronously.
  • Update FSharp.Core surface area baselines and release notes; extend XML docs for RunSynchronously/RunSynchronouslyImmediate.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs Adds unit tests for RunSynchronouslyImmediate and contrasts with RunSynchronously.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.release.bsl Records new public surface area entry for RunSynchronouslyImmediate.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.debug.bsl Records new public surface area entry for RunSynchronouslyImmediate.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.release.bsl Records new public surface area entry for RunSynchronouslyImmediate.
tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.debug.bsl Records new public surface area entry for RunSynchronouslyImmediate.
src/FSharp.Core/async.fsi Adds XML docs for the new API and revises RunSynchronously docs to reference it.
src/FSharp.Core/async.fs Implements the new API by exposing an “immediate” synchronous runner and refactoring RunSynchronously internals.
docs/release-notes/.FSharp.Core/11.0.100.md Adds release note entry for Async.RunSynchronouslyImmediate.
Comments suppressed due to low confidence (1)

tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs:527

  • This test uses a raw Thread but doesn’t capture exceptions from the thread body. Any unexpected exception inside the thread (including from Async.RunSynchronously) will be unhandled and may crash the test process rather than reporting a normal xUnit failure. Capture exceptions in the thread and rethrow/assert after Join.
        let t = Thread(fun () ->
            callerThreadId <- Thread.CurrentThread.ManagedThreadId
            async { runSyncThreadId <- Thread.CurrentThread.ManagedThreadId }
            |> Async.RunSynchronously
            async { immThreadId <- Thread.CurrentThread.ManagedThreadId }
            |> Async.RunSynchronouslyImmediate)

Comment thread tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs Outdated
Comment thread src/FSharp.Core/async.fsi Outdated
Comment thread src/FSharp.Core/async.fsi Outdated
Comment thread src/FSharp.Core/async.fsi Outdated
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 25, 2026
Comment thread src/FSharp.Core/async.fsi
Comment on lines +90 to +104
/// <summary><p>Runs the asynchronous computation, starting and blocking on the
/// calling thread regardless of <see cref="P:System.Threading.SynchronizationContext.Current"/> being non-
/// <c>null</c> or <see cref="P:System.Threading.Thread.IsThreadPoolThread"/> being <c>false</c>.</p>
/// <p>Any exception raised by the computation is propagated to the caller, with a stack trace bearing
/// caller frames for exceptions raised before the first asynchronous suspension.</p>
/// <p>Warning: may cause deadlock if called on a UI thread.</p>
/// </summary>
///
/// The timeout parameter is given in milliseconds. A value of -1 is equivalent to
/// <see cref="F:System.Threading.Timeout.Infinite"/>.
/// <remarks>
/// <p>Warning: this method hard-blocks the calling thread for the duration of the computation,
/// including threads that have a non-<c>null</c>
/// <see cref="P:System.Threading.SynchronizationContext.Current"/> such as UI threads. Calling it
/// from a UI thread will make the UI unresponsive and risks deadlock if any continuation in the
/// computation needs to be dispatched back to that context.
/// </p>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary and remarks feel very much like.

Maybe summary = launches on the calling thread and what it is good for
remarks - the exact conditions and risks

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@T-Gro see #19804 (comment) - I'll rewrite the xmldoc based on what we decide. I agree that there should be less/no duplication, and summary should mention only risk (deadlock), especially if only key reason to use new fn is to get a direct callstack in a debugger.

Comment thread src/FSharp.Core/async.fsi
/// </code>
/// Prints "A", "B" immediately, then "C", "D" in 1 second. result is set to 17.
/// Prints <c>"A"</c>, <c>"B"</c> immediately (on the calling thread),
/// then (from a continuation thread), after about one second, <c>"C"</c>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the example commentary ((from a continuation thread)) swapped between the two versions?

Copy link
Copy Markdown
Author

@bartelink bartelink May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check after the rewrite when my comment in main thread is resolved - lots of cut and paste going on. Assuming the key diff is deemed to be less noise in the stack trace, the examples will emphasize that the first block prior to suspension runs on calling thread vs a threadpool thread

@bartelink
Copy link
Copy Markdown
Author

@T-Gro I've implemented as per OP of fsharp/fslang-suggestions#1042

Firstly, in a debugger context a first chance exception breakpoint has more direct causation vs having to disentangle an adjacent thread waiting, which tooling may or may not be able to convey unaided.

However, while the exception stack trace has less noise, it's not significantly better AFAICT:

image

It seems the TL;DR value prop is:

  • in a debugger breakpoint there's an obvious call stack
  • stack traces have 2/3 less noise layers
  • default perf is better as no egregious thread hops (though for many real world cases, there may be a root RunSynchronously that pays the hop price and then nested calls don't pay?)

But its not a slam dunk as:

  • potential deadlocks
  • confusion about a longstanding thing that people's fingers and thousands of tutorials had

This is making me think I should dial back the xmldoc from where I have it trying to set a "use the new RunSynchronouslyImmediate in FSI from now on" vibe (as I've tried to do with AwaitTask vs Await where it is a slam dunk)

cc @majocha @dsyme

Comment thread src/FSharp.Core/async.fs
|> unfake

[<DebuggerHidden>]
let RunSynchronouslyImmediate2 computation (cancellationToken: CancellationToken) =
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@T-Gro If I put this body (which gives a shorter callstack than the original RunSynchronouslyImmediate impl at L1141) into RunSynchronouslyImmediate, the tests also pass but I guess there is more risk of regressions due to changing a very hot path.

Let me know if you'd prefer for RunSynchronouslyImmediate2 and RunSynchronouslyImmediate to live on side by side as the code is now, or whether you're ok with me collapsing the two into this impl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants